Emit sentFrame and receivedFrame events from endpoint. #135
Conversation
Want to be able to inspect frames as the come and go. Added an onEndpoint parameter to Agent.request() and EndPoint(). Needed onEndpoint because after constructor for EndPoint has returned some frames have already been sent, and would be missed by any event handler. Also added onSentFrame and onReceivedFrame parameters to Connection() constructor for same reason. Refactored the constructor for EndPoint and Connection to take a config object instead of a list of parameters. The parameter list on the constructor was getting pretty long. Added test for sentFrame, and receiveFrame. Updated existing tests to handle new constructor for EndPoint and Connection. Conflicts: lib/http.js
|
I also updated the inline documentation. Is the wiki just a copy and paste of the code comments, or is there another system which automatically updates the wiki from code comments? Also this pull request contains breaking changes (constructor to Endpoint and Connection). Should increment the Major version. |
|
I'm not convinced the breaking API changes need to happen - it's just as easy to add arguments (which will default to undefined) without breaking backwards compat (and the argument lists really aren't that long). Other than that, this seems pretty OK on the surface of it. I'll take a more thorough look once the breaking changes have been reverted. As to the wiki/pages for this - those are generated from the inline comments. It's automated in that it's not a strict copy/paste, but it's manual in that someone has to go in and run the doc generation (and update the gh-pages branch). I'll get to that one of these days :) |
|
I like having an options object because having a mix of optional and required options becomes a mess to handle. For example, an optional onSocket parameter was added to the constructor. We would have to use What if we leave the required arguments be function parameters, and add an optional So it would end up looking like: var options = {
myOnSocket: function(socket){}
};
Endpoint(myLog, 'CLIENT', mySetting, options); // ok
Endpoint(myLog, 'CLIENT', mySetting); // okThis would not break backwards compatibility, and allow for future expansion of arguments. What do you think? |
|
Hrm... that feels even less desirable, to me - mixed metaphors in a way, with some arguments being explicit/positional, and some being in a (positional) dict. I'm tempted to go one of two routes here - either we just hold this patch back until more API breaking changes come along that we can batch this with (I would prefer to keep API churn to a minimum now that h2 is complete), or do the time-honored trick of looking at the first argument to the function, if it's a dict, treat it like the function was called in this new way (with an options dict and nothing else), and otherwise, treat it like it was called using the current API. I'm leaning towards the latter, because I do see some value in having this functionality, though I also feel like there should be a better way to manage this entirely, in order to avoid synchronous callbacks. I'll have to think on it some more. |
Want to be able to inspect frames as the come and go. Added an onEndpoint parameter to Agent.request() and EndPoint(). Needed onEndpoint because after constructor for EndPoint has returned some frames have already been sent, and would be missed by any event handler. Also added onSentFrame and onReceivedFrame parameters to Connection() constructor for same reason.
Refactored the constructor for EndPoint and Connection to take a config object instead of a list of parameters. The parameter list on the constructor was getting pretty long.
Added test for sentFrame, and receiveFrame. Updated existing tests to handle new constructor for EndPoint and Connection.